CNF-23374: Migrate away from deprecated ioutil - #202

pkg/oauthserver/oauth_apiserver_test.go (1)

29-101: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Close os.CreateTemp file handles before os.WriteFile (prevents leaks/flaky cleanup).

In each test, os.CreateTemp returns an open *os.File, but it’s never closed before writing via os.WriteFile(tmpfile.Name(), ...). This is a resource leak and can cause flaky failures on platforms that restrict deleting/opening temp files (commonly Windows), since os.Remove is deferred.

🔧 Proposed fix

 func TestGetInvalidSessionSecretsFile(t *testing.T) {
-	tmpfile, err := os.CreateTemp("", "invalid.yaml")
+	tmpfile, err := os.CreateTemp("", "invalid.yaml")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	defer os.Remove(tmpfile.Name())
+	name := tmpfile.Name()
+	if err := tmpfile.Close(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	defer os.Remove(name)

-	if err := os.WriteFile(tmpfile.Name(), []byte("invalid content"), os.FileMode(0600)); err != nil {
+	if err := os.WriteFile(name, []byte("invalid content"), os.FileMode(0600)); err != nil {
 		t.Fatal(err)
 	}

-	_, err = getSessionSecrets(tmpfile.Name())
+	_, err = getSessionSecrets(name)
 	if err == nil {
 		t.Errorf("Expected error, got none")
 	}
 }

 func TestGetEmptySessionSecretsFile(t *testing.T) {
-	tmpfile, err := os.CreateTemp("", "empty.yaml")
+	tmpfile, err := os.CreateTemp("", "empty.yaml")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	defer os.Remove(tmpfile.Name())
+	name := tmpfile.Name()
+	if err := tmpfile.Close(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	defer os.Remove(name)

 	secrets := &osinv1.SessionSecrets{
 		Secrets: []osinv1.SessionSecret{},
 	}
 ...
-	if err := os.WriteFile(tmpfile.Name(), []byte(yaml), os.FileMode(0600)); err != nil {
+	if err := os.WriteFile(name, []byte(yaml), os.FileMode(0600)); err != nil {
 		t.Fatal(err)
 	}

-	_, err = getSessionSecrets(tmpfile.Name())
+	_, err = getSessionSecrets(name)
 	if err == nil {
 		t.Errorf("Expected error, got none")
 	}
 }

 func TestGetValidSessionSecretsFile(t *testing.T) {
-	tmpfile, err := os.CreateTemp("", "valid.yaml")
+	tmpfile, err := os.CreateTemp("", "valid.yaml")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	defer os.Remove(tmpfile.Name())
+	name := tmpfile.Name()
+	if err := tmpfile.Close(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	defer os.Remove(name)
 ...
-	if err := os.WriteFile(tmpfile.Name(), []byte(yaml), os.FileMode(0600)); err != nil {
+	if err := os.WriteFile(name, []byte(yaml), os.FileMode(0600)); err != nil {
 		t.Fatal(err)
 	}

-	readSecrets, err := getSessionSecrets(tmpfile.Name())
+	readSecrets, err := getSessionSecrets(name)
 	if err != nil {
 		t.Errorf("Unexpected error: %v", err)
 	}
 	if !reflect.DeepEqual(readSecrets, expectedSecrets) {
 		t.Errorf("Unexpected %v, got %v", expectedSecrets, readSecrets)
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/oauthserver/oauth_apiserver_test.go` around lines 29 - 101, Each test
(TestGetInvalidSessionSecretsFile, TestGetEmptySessionSecretsFile,
TestGetValidSessionSecretsFile) opens a temp file with os.CreateTemp and then
calls os.WriteFile on its name without closing the *os.File handle, which can
leak resources and cause flaky behavior on some platforms; fix by calling
tmpfile.Close() (or defer tmpfile.Close()) immediately after successful
os.CreateTemp returns and before calling os.WriteFile/tmpfile.Name(), ensuring
the file handle is released before getSessionSecrets reads it.

pkg/server/login/login_test.go (1)

248-257: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Don’t ignore io.ReadAll errors in tests.

The test discards the error from io.ReadAll(resp.Body) (data, _ := ...). If reading fails, you’ll get misleading assertion failures.

💡 Suggested fix

-			data, _ := io.ReadAll(resp.Body)
-			body := string(data)
+			data, err := io.ReadAll(resp.Body)
+			if err != nil {
+				t.Fatalf("%s: failed to read response body: %v", k, err)
+			}
+			body := string(data)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/server/login/login_test.go` around lines 248 - 257, The test currently
ignores the error returned by io.ReadAll(resp.Body) in the block handling
testCase.ExpectContains; change the code to capture and check the error (e.g.
data, err := io.ReadAll(resp.Body)), and if err != nil call t.Fatalf or t.Errorf
with the error (including context like which test case key `k`) instead of
proceeding to string conversion and substring assertions; update references in
this block (resp.Body read, variable `body`, and the t.Errorf checks for missing
substrings) so assertions only run when the read succeeded.

pkg/server/selectprovider/selectprovider_test.go (1)

98-107: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Don’t ignore io.ReadAll errors in selectprovider test.

data, _ := io.ReadAll(resp.Body) drops the error. If reading fails, your strings.Contains assertions may report missing substrings for the wrong reason.

💡 Suggested fix

-			data, _ := io.ReadAll(resp.Body)
-			body := string(data)
+			data, err := io.ReadAll(resp.Body)
+			if err != nil {
+				t.Fatalf("%s: failed to read response body: %v", k, err)
+			}
+			body := string(data)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/server/selectprovider/selectprovider_test.go` around lines 98 - 107, The
test is ignoring the error from io.ReadAll(resp.Body) which can mask I/O
failures; change the read to capture the error (replace the underscore) and if
err != nil call t.Fatalf or t.Errorf (including the test case key `k` and the
error) before using `body`, then continue with the existing `strings.Contains`
checks that reference `testCase.ExpectContains` and `t.Errorf` for missing
substrings.